From f9840b50cac93eb29c587515ae38fa35a8247c51 Mon Sep 17 00:00:00 2001 From: tsteven4 <13596209+tsteven4@users.noreply.github.com> Date: Wed, 18 Mar 2020 12:50:04 -0600 Subject: [PATCH] modernize radius filter ... (#511) * modernize radius filter ... and fix a memory leak when maxcount option discards wpts. * radius loop safety. * allow any Comparable with Waypoint/Route List sort. * use static_cast instead of reinterpret_cast to recover original type from void*. --- arcdist.cc | 4 +- defs.h | 35 +++++++++---- radius.cc | 122 +++++++++++++++---------------------------- radius.h | 1 - reference/radius.csv | 2 + route.cc | 17 ------ testo.d/radius.test | 3 +- waypt.cc | 11 ---- 8 files changed, 73 insertions(+), 122 deletions(-) diff --git a/arcdist.cc b/arcdist.cc index 5eee2a6ca..e75654a17 100644 --- a/arcdist.cc +++ b/arcdist.cc @@ -56,7 +56,7 @@ void ArcDistanceFilter::arcdist_arc_disp_wpt_cb(const Waypoint* arcpt2) if (waypointp->extra_data) { ed = (extra_data*) waypointp->extra_data; } else { - ed = (extra_data*) xcalloc(1, sizeof(*ed)); + ed = new extra_data; ed->distance = BADVAL; } if (ed->distance == BADVAL || projectopt || ed->distance >= pos_dist) { @@ -206,7 +206,7 @@ void ArcDistanceFilter::process() qPrintable(wp->shortname), ed->distance, wp->latitude, wp->longitude); } } - xfree(ed); + delete ed; } } if (global_opts.verbose_status > 0) { diff --git a/defs.h b/defs.h index f5dc7ffdc..22d0ec6a3 100644 --- a/defs.h +++ b/defs.h @@ -19,6 +19,7 @@ #ifndef DEFS_H_INCLUDED_ #define DEFS_H_INCLUDED_ +#include // for sort, stable_sort #include // for M_PI #include // for va_list #include // for NULL, nullptr_t, size_t @@ -549,8 +550,6 @@ using waypt_cb = void (*)(const Waypoint*); class WaypointList : private QList { public: - using Compare = bool (*)(const Waypoint*, const Waypoint*); - void waypt_add(Waypoint* wpt); // a.k.a. append(), push_back() void add_rte_waypt(int waypt_ct, Waypoint* wpt, bool synth, const QString& namepart, int number_digits); // FIXME: Generally it is inefficient to use an element pointer or reference to define the element to be deleted, use iterator instead, @@ -565,7 +564,8 @@ public: void copy(WaypointList** dst) const; void restore(WaypointList* src); void swap(WaypointList& other); - void sort(Compare cmp); + template + void sort(Compare cmp) {std::stable_sort(begin(), end(), cmp);} template void waypt_disp_session(const session_t* se, T cb); @@ -610,7 +610,13 @@ void waypt_append(WaypointList* src); void waypt_backup(WaypointList** head_bak); void waypt_restore(WaypointList* head_bak); void waypt_swap(WaypointList& other); -void waypt_sort(WaypointList::Compare cmp); +template +void waypt_sort(Compare cmp) +{ + extern WaypointList* global_waypoint_list; + + global_waypoint_list->sort(cmp); +} void waypt_add_url(Waypoint* wpt, const QString& link, const QString& url_link_text); void waypt_add_url(Waypoint* wpt, const QString& link, @@ -712,8 +718,6 @@ using route_trl = void (*)(const route_head*); class RouteList : private QList { public: - using Compare = bool (*)(const route_head*, const route_head*); - int waypt_count() const; void add_head(route_head* rte); // a.k.a. append(), push_back() // FIXME: Generally it is inefficient to use an element pointer or reference to define the element to be deleted, use iterator instead, @@ -729,7 +733,8 @@ public: void copy(RouteList** dst) const; void restore(RouteList* src); void swap(RouteList& other); - void sort(Compare cmp); + template + void sort(Compare cmp) {std::sort(begin(), end(), cmp);} template void disp_all(T1 rh, T2 rt, T3 wc); template @@ -795,11 +800,23 @@ void track_append(RouteList* src); void route_backup(RouteList** head_bak); void route_restore(RouteList* head_bak); void route_swap(RouteList& other); -void route_sort(RouteList::Compare cmp); +template +void route_sort(Compare cmp) +{ + extern RouteList* global_route_list; + + global_route_list->sort(cmp); +} void track_backup(RouteList** head_bak); void track_restore(RouteList* head_bak); void track_swap(RouteList& other); -void track_sort(RouteList::Compare cmp); +template +void track_sort(Compare cmp) +{ + extern RouteList* global_track_list; + + global_track_list->sort(cmp); +} computed_trkdata track_recompute(const route_head* trk); template diff --git a/radius.cc b/radius.cc index 49ae52e64..e71f25594 100644 --- a/radius.cc +++ b/radius.cc @@ -19,57 +19,29 @@ */ -#include // for atof, atoi, qsort, strtod +#include // for atoi, strtod #include // for QString -#include // for foreach +#include // for qAsConst, QAddConst<>::Type, foreach -#include "defs.h" +#include "defs.h" // for Waypoint, waypt_del, route_add_head, route_add_wpt, route_head, waypt_add, waypt_count, xcalloc, xfree, kMilesPerKilometer #include "radius.h" #include "grtcirc.h" // for RAD, gcdist, radtomiles + #if FILTERS_ENABLED double RadiusFilter::gc_distance(double lat1, double lon1, double lat2, double lon2) { - return gcdist( - RAD(lat1), - RAD(lon1), - RAD(lat2), - RAD(lon2) - ); -} - -int RadiusFilter::dist_comp(const void* a, const void* b) -{ - const Waypoint* x1 = *(Waypoint**)a; - const Waypoint* x2 = *(Waypoint**)b; - auto* x1e = (extra_data*) x1->extra_data; - auto* x2e = (extra_data*) x2->extra_data; - - if (x1e->distance > x2e->distance) { - return 1; - } - if (x1e->distance < x2e->distance) { - return -1; - } - return 0; - + return radtomiles(gcdist(RAD(lat1), RAD(lon1), RAD(lat2), RAD(lon2))); } void RadiusFilter::process() { - Waypoint** comp; - int i, wc; - route_head* rte_head = nullptr; + // waypt_del may modify container. foreach (Waypoint* waypointp, *global_waypoint_list) { - double dist = gc_distance(waypointp->latitude, - waypointp->longitude, - home_pos->latitude, - home_pos->longitude); - - /* convert radians to float point statute miles */ - dist = radtomiles(dist); + double dist = gc_distance(waypointp->latitude, waypointp->longitude, + home_pos->latitude, home_pos->longitude); if ((dist >= pos_dist) == (exclopt == nullptr)) { waypt_del(waypointp); @@ -77,70 +49,60 @@ void RadiusFilter::process() continue; } - auto* ed = (extra_data*) xcalloc(1, sizeof(extra_data)); + auto* ed = new extra_data; ed->distance = dist; waypointp->extra_data = ed; } - wc = waypt_count(); - - comp = (Waypoint**) xcalloc(wc, sizeof(*comp)); - - i = 0; - - /* - * Create an array of remaining waypoints, popping them off the - * master queue as we go. This gives us something reasonable - * for qsort. - */ - - foreach (Waypoint* wp, *global_waypoint_list) { - comp[i] = wp; - waypt_del(wp); - i++; - } - - if (!nosort) { - qsort(comp, wc, sizeof(Waypoint*), dist_comp); + if (nosort == nullptr) { + auto dist_comp_lambda = [](const Waypoint* a, const Waypoint* b)->bool { + const auto* aed = static_cast(a->extra_data); + const auto* bed = static_cast(b->extra_data); + return aed->distance < bed->distance; + }; + waypt_sort(dist_comp_lambda); } - if (routename) { + route_head* rte_head = nullptr; + if (routename != nullptr) { rte_head = new route_head; rte_head->rte_name = routename; route_add_head(rte_head); } /* - * The comp array is now sorted by distance. As we run through it, - * we push them back onto the master wp list, letting us pass them - * on through in the modified order. + * Create an list of remaining waypoints. + * Delete them, add them to the global waypoint list, or add them + * to a new route. */ - for (i = 0; i < wc; i++) { - Waypoint* wp = comp[i]; - xfree(wp->extra_data); + WaypointList comp; + waypt_swap(comp); + + int i = 0; + for (Waypoint* wp : qAsConst(comp)) { + delete static_cast(wp->extra_data); wp->extra_data = nullptr; - if (maxctarg && i >= maxct) { - continue; - } - if (routename) { - route_add_wpt(rte_head, wp); + if ((maxctarg != nullptr) && (i >= maxct)) { + delete wp; } else { - waypt_add(wp); + if (routename != nullptr) { + route_add_wpt(rte_head, wp); + } else { + waypt_add(wp); + } } + ++i; } - - xfree(comp); } void RadiusFilter::init() { - char* fm; - pos_dist = 0; - if (distopt) { + if (distopt != nullptr) { + char* fm; pos_dist = strtod(distopt, &fm); if ((*fm == 'k') || (*fm == 'K')) { @@ -149,7 +111,7 @@ void RadiusFilter::init() } } - if (maxctarg) { + if (maxctarg != nullptr) { maxct = atoi(maxctarg); } else { maxct = 0; @@ -157,11 +119,11 @@ void RadiusFilter::init() home_pos = new Waypoint; - if (latopt) { - home_pos->latitude = atof(latopt); + if (latopt != nullptr) { + home_pos->latitude = strtod(latopt, nullptr); } - if (lonopt) { - home_pos->longitude = atof(lonopt); + if (lonopt != nullptr) { + home_pos->longitude = strtod(lonopt, nullptr); } } diff --git a/radius.h b/radius.h index ab1b77e41..75f5a5ca3 100644 --- a/radius.h +++ b/radius.h @@ -89,7 +89,6 @@ private: }; double gc_distance(double lat1, double lon1, double lat2, double lon2); - static int dist_comp(const void* a, const void* b); }; #endif // FILTERS_ENABLED diff --git a/reference/radius.csv b/reference/radius.csv index 4095cad3e..f5c656709 100644 --- a/reference/radius.csv +++ b/reference/radius.csv @@ -1 +1,3 @@ 35.97203, -87.13470, Mountain Bike Heaven by susy1313 +36.05750, -86.89200, GittyUp by JoGPS / Warner Parks +36.08280, -86.86728, Inlighting by JoGPS / Warner Parks diff --git a/route.cc b/route.cc index 3070ea026..2cde1145c 100644 --- a/route.cc +++ b/route.cc @@ -211,12 +211,6 @@ route_swap(RouteList& other) global_route_list->swap(other); } -void -route_sort(RouteList::Compare cmp) -{ - global_route_list->sort(cmp); -} - void track_backup(RouteList** head_bak) { @@ -235,12 +229,6 @@ track_swap(RouteList& other) global_track_list->swap(other); } -void -track_sort(RouteList::Compare cmp) -{ - global_track_list->sort(cmp); -} - /* * This really makes more sense for tracks than routes. * Run over all the trackpoints, computing heading (course), speed, and @@ -510,8 +498,3 @@ void RouteList::swap(RouteList& other) *this = other; other = tmp_list; } - -void RouteList::sort(Compare cmp) -{ - std::sort(begin(), end(), cmp); -} diff --git a/testo.d/radius.test b/testo.d/radius.test index 70c3a400c..41c19c788 100644 --- a/testo.d/radius.test +++ b/testo.d/radius.test @@ -4,7 +4,6 @@ # rm -f ${TMPDIR}/radius.csv gpsbabel -i geo -f ${REFERENCE}/geocaching.loc \ - -x radius,lat=35.9720,lon=-87.1347,distance=14.7 \ + -x radius,lat=35.9720,lon=-87.1347,distance=20.0,maxcount=3 \ -o csv -F ${TMPDIR}/radius.csv compare ${TMPDIR}/radius.csv ${REFERENCE} - diff --git a/waypt.cc b/waypt.cc index e7e942b86..15cfe3493 100644 --- a/waypt.cc +++ b/waypt.cc @@ -197,12 +197,6 @@ waypt_swap(WaypointList& other) global_waypoint_list->swap(other); } -void -waypt_sort(WaypointList::Compare cmp) -{ - global_waypoint_list->sort(cmp); -} - void waypt_add_url(Waypoint* wpt, const QString& link, const QString& url_link_text) { @@ -745,8 +739,3 @@ void WaypointList::swap(WaypointList& other) *this = other; other = tmp_list; } - -void WaypointList::sort(Compare cmp) -{ - std::stable_sort(begin(), end(), cmp); -} -- 2.30.2